Skip to content

Conversation

@breskeby
Copy link
Contributor

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.

@breskeby breskeby added >non-issue :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 v9.1.0 v8.16.6 v8.17.4 labels Feb 26, 2025
@breskeby breskeby self-assigned this Feb 26, 2025
@breskeby breskeby force-pushed the fix-extended-plugin-lookup branch from c79bac8 to ca0117d Compare February 26, 2025 11:55
@breskeby breskeby force-pushed the fix-extended-plugin-lookup branch from ed1832e to 9759f49 Compare March 13, 2025 09:49
@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Mar 13, 2025
@breskeby breskeby marked this pull request as ready for review March 13, 2025 11:24
@breskeby breskeby requested review from a team as code owners March 13, 2025 11:24
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@jozala jozala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

classname ='org.elasticsearch.aggregations.AggregationsPlugin'
extendedPlugins = ['lang-painless']
extendedPluginProjects {
"lang-painless" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax seems a bit verbose. Is there a way we could use just the project path, and resolve something from the project at runtime to determine the plugin name? Most of the time this is just the project name, so having to specify it twice along with the extra syntax feels like unnecessary boilerplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that unfortunately the naming is not deterministic across the different plugins. At the moment the actual name is declared in the build script only. We could resolve the name maybe by resolving the generated plugin properties, but for now I prefer this little bit boilerplate in the definition over the constant cost of trying to be clever here and resolve that dynamically.

Looking at this again, I just realize we might have a better way to move forward here as we only need the the project path atm to configure a potential test cluster automatically. Given we use less and less the test cluster plugin we probably tweak this to not generally change the plugin declaration but just do the module dependency in the test clusters explicitly.

puh, let me revisit this again 🤡

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjernst turns out alls this was basically not required in our current codebase. see https://github.com/elastic/elasticsearch/pull/123461/files#r1995753719

@breskeby breskeby force-pushed the fix-extended-plugin-lookup branch from 484b12a to dfe7d45 Compare March 14, 2025 15:02
if (isModule == false || isXPackModule) {
addNoticeGeneration(project, extension);
}
project.afterEvaluate(p -> {
Copy link
Contributor Author

@breskeby breskeby Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this was only used by internal plugins that

a) still rely on test cluster plugin,
b) extend other plugins
c) do not use default distribution

which is actually ZERO

@breskeby breskeby force-pushed the fix-extended-plugin-lookup branch from c3ab3e4 to ce2aef6 Compare March 14, 2025 15:13
@breskeby breskeby requested a review from rjernst March 14, 2025 15:19
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. FWIW it would be nice to not have to specify dependencies twice, once in the plugin properties block, and again under dependencies. But this is good on its own.

@breskeby
Copy link
Contributor Author

LGTM. FWIW it would be nice to not have to specify dependencies twice, once in the plugin properties block, and again under dependencies. But this is good on its own.

true. we would need some stronger modelling here for this.
At the moment we want to put the focus first on cleaning our plugins and scripts from deprecated / discouraged api usages and accept some downsides in some verbosity for now I think.

@breskeby breskeby merged commit 7f722e9 into elastic:main Mar 17, 2025
14 of 17 checks passed
breskeby added a commit to breskeby/elasticsearch that referenced this pull request Mar 17, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
breskeby added a commit to breskeby/elasticsearch that referenced this pull request Mar 17, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
breskeby added a commit to breskeby/elasticsearch that referenced this pull request Mar 17, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
breskeby added a commit to breskeby/elasticsearch that referenced this pull request Mar 17, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
breskeby added a commit to breskeby/elasticsearch that referenced this pull request Mar 17, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0
8.16
8.17

@breskeby breskeby deleted the fix-extended-plugin-lookup branch March 17, 2025 07:39
breskeby added a commit that referenced this pull request Mar 17, 2025
…jects (#123461) (#124993)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
elasticsearchmachine pushed a commit that referenced this pull request Mar 17, 2025
…jects (#123461) (#124995)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
elasticsearchmachine pushed a commit that referenced this pull request Mar 17, 2025
…jects (#123461) (#124992)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
elasticsearchmachine pushed a commit that referenced this pull request Mar 17, 2025
…jects (#123461) (#124991)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Mar 17, 2025
* main: (95 commits)
  Mute org.elasticsearch.datastreams.lifecycle.DataStreamLifecycleServiceIT testLifecycleAppliedToFailureStore elastic#124999
  Merge template mappings properly during validation (elastic#124784)
  [Build] Rework internal build plugin plugin to work with Isolated Projects (elastic#123461)
  [Build] Require reason for usesDefaultDistribution (elastic#124707)
  Mute org.elasticsearch.packaging.test.DockerTests test011SecurityEnabledStatus elastic#124990
  Mute org.elasticsearch.xpack.ilm.TimeSeriesDataStreamsIT testRolloverAction elastic#124987
  Mute org.elasticsearch.packaging.test.BootstrapCheckTests test10Install elastic#124957
  Mute org.elasticsearch.integration.DataStreamLifecycleServiceRuntimeSecurityIT testRolloverLifecycleAndForceMergeAuthorized elastic#124978
  Mute org.elasticsearch.xpack.esql.action.CrossClusterAsyncQueryStopIT testStopQuery elastic#124977
  Mute org.elasticsearch.xpack.esql.action.CrossClusterAsyncQueryStopIT testStopQueryLocal elastic#121672
  Mention zero-window state in networking docs (elastic#124967)
  Remove remoteAddress field from TransportResponse (elastic#120016)
  Include failures in partial response (elastic#124929)
  Prevent work starvation bug if using scaling EsThreadPoolExecutor with core pool size = 0  (elastic#124732)
  Re-enable analysis stemmer test (elastic#124961)
  Mute org.elasticsearch.xpack.esql.action.CrossClusterAsyncQueryStopIT testStopQueryLocalNoRemotes elastic#124959
  ESQL: Catch parsing exception (elastic#124958)
  ESQL: Improve error message for ( and [ (elastic#124177)
  Mute org.elasticsearch.xpack.esql.qa.single_node.EsqlSpecIT test {lookup-join.MvJoinKeyFromRow SYNC} elastic#124951
  Mute org.elasticsearch.datastreams.lifecycle.DataStreamLifecycleServiceIT testErrorRecordingOnRetention elastic#124950
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
#	server/src/test/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldTypeTests.java
omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025
…jects (elastic#123461)

This fixes a general flaw in our build logic where we reach out to configurations of other projects.
This is not best practice and breaks future initiatives like IsolatedProjects that allow parallel configuration
of subprojects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Delivery/Build Build or test infrastructure >non-issue serverless-linked Added by automation, don't add manually Team:Delivery Meta label for Delivery team v8.16.6 v8.17.4 v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants